This means the threads will stop right after they have started. I would prefer the pattern that is used in some other tests in this file: set a constant NUM_LOOPS (determined so that the test < 0.1 seconds, but there still is a decent number of mutations)

Here so is a borrowed reference to si->si_set. But si->si_set can be cleared in setiter_iternext (if the iterator is exhausted) outside the critical section.

This is a different mechanism than the corresponding issue, so maybe something to address in another PR. But solving both together is something to consider.

Good catch. Thanks a lot.

This might work for setiter_len, but setiter_iternext itself is not yet thread safe (also because of setting si->si_set to zero).

For several other iterations the approach is to keep the reference si->si_set , but use another attribute to signal exhaustion of the iterator. For example for itertools.cycle or the reversed operator.

Note: I tried creating a minimal example where concurrent iteration fails, but I have succeeded yet (the example does not crash, although I have not run thread sanitizer on it yet)

import unittest
from threading import Thread, Barrier


class TestSetIter(unittest.TestCase):
    def test_set_iter(self):
        """Test concurrent iteration over a set"""

        NUM_LOOPS = 10_000
        NUM_THREADS = 4
        

        for ii in range(NUM_LOOPS):
            if ii % 1000 ==0:
                print(f'test_set_iter {ii}')
            barrier = Barrier(NUM_THREADS)
            
            # make sure the underlying set is unique referenced by the iterator
            iterator = iter(set((1,2,))) 
            
            def worker():
                barrier.wait()
                while True:
                    iterator.__length_hint__()
                    try:
                        next(iterator)
                    except StopIteration:
                        break

                
            threads = [Thread(target=worker) for _ in range(NUM_THREADS)]
            for t in threads:
                t.start()
            for t in threads:
                t.join()
                
            assert iterator.__length_hint__()==0

if __name__ == "__main__":
    unittest.main()

Thank you. I think your points make a lot of sense, and I really appreciate the two links you shared—they helped me get a more complete picture of the iterator-related data race.
I’ll try to construct the case you mentioned under a TSan environment.
If it turns out to be appropriate, we can address it fully in this PR, that would be great. Of course, this will take some time.

Yes, we should fix this like we have fixed others and as Sam suggested only clear the associated set in non-free-threading builds. The current code is incorrect because it uses try incref which can fail spuriously if the set object is not marked to enable try incref.

I think we should follow the pattern that we use in other iterators: don't clear si->si_set when the iterator is exhausted in the free-threaded build.

That will keep other things simpler.

Please use the same style for starting and stopping threads as the other tests in this file (e.g. test_contains_hash_mutate)

(the exhausted variable is then not needed any more I believe)

You cannot directly return as it would skip ending the critical section

On the normal build the macro will expand to si->si_pos = i + 1;

This (and some other places) should also be atomic?

The value -1 written here could be overwritten by a concurrent thread (at line 1155). Which means that over exhaustion of the set iterator it is restored back to life. This does not lead to overflows or other issues (afaic), but is a bit odd behaviour.

I think in the normal build you still have to do si->si_set = NULL;, otherwise the si->si_set is decref'ed again in setiter_dealloc.